Skip to content

chore: update pre-commit#184

Open
danielsirakov wants to merge 13 commits into
diffpy:mainfrom
danielsirakov:update_pre-commit
Open

chore: update pre-commit#184
danielsirakov wants to merge 13 commits into
diffpy:mainfrom
danielsirakov:update_pre-commit

Conversation

@danielsirakov

@danielsirakov danielsirakov commented May 31, 2026

Copy link
Copy Markdown

@stevenhua0320, ready to review

@stevenhua0320

Copy link
Copy Markdown
Contributor

@danielsirakov same here.

@codecov

codecov Bot commented May 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.63%. Comparing base (a9eae82) to head (8977ae6).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #184   +/-   ##
=======================================
  Coverage   71.63%   71.63%           
=======================================
  Files          25       25           
  Lines        3437     3437           
=======================================
  Hits         2462     2462           
  Misses        975      975           
Files with missing lines Coverage Δ
tests/test_builder.py 100.00% <ø> (ø)
tests/test_characteristicfunctions.py 12.26% <ø> (ø)
tests/test_constraint.py 96.96% <ø> (ø)
tests/test_contribution.py 99.51% <ø> (ø)
tests/test_equation.py 100.00% <ø> (ø)
tests/test_fitrecipe.py 99.76% <ø> (ø)
tests/test_literals.py 99.17% <ø> (ø)
tests/test_objcrystparset.py 10.27% <ø> (ø)
tests/test_profile.py 99.33% <ø> (ø)
tests/test_recipeorganizer.py 99.69% <ø> (ø)
... and 4 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sbillinge

Copy link
Copy Markdown
Contributor

For some weird reason this is deleting blank lines after the docstring. I asked claude and it seems as if this is a bug in v1.7.8 of docformatter which people have requested fixes for. Please could you try again (let's close this PR so we don't merge all the back and forth history) pinning to 1.7.7 and see what happens?

@stevenhua0320

Copy link
Copy Markdown
Contributor

@danielsirakov could you revisit this one as we have an agreement that deleting line form docformatter is a right fix. So, let's do a review to this one and make sure that everything is right. Thanks!

@sbillinge

Copy link
Copy Markdown
Contributor

The files look Ok. We just need to check if this version is passing pre-commit

@stevenhua0320

Copy link
Copy Markdown
Contributor

@danielsirakov Could you take a look for the failure of black and docformatter? Seems there is a conflict in the formatting of the docstring here. If it is the case, we could do a reformatting as before to get around this. Thanks for your effort.

@danielsirakov

Copy link
Copy Markdown
Author

@danielsirakov Could you take a look for the failure of black and docformatter? Seems there is a conflict in the formatting of the docstring here. If it is the case, we could do a reformatting as before to get around this. Thanks for your effort.

Sounds good, I'll take a look after I make the last few edits to diffpy.structure

…rfit into update_pre-commit

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
@danielsirakov

Copy link
Copy Markdown
Author

I was able to bypass the docformatter and black conflict by changing triple quotes to just using double quotes in 3 files. Let me know if this is an appropriate workaround.

@stevenhua0320, ready to review

@stevenhua0320

Copy link
Copy Markdown
Contributor

@danielsirakov I think we have one left for the conflict here:
tests/test_characteristicfunctions.py
you can check what is conflicting by running it on local and see how we gonna fix this.

@danielsirakov

Copy link
Copy Markdown
Author

@danielsirakov I think we have one left for the conflict here: tests/test_characteristicfunctions.py you can check what is conflicting by running it on local and see how we gonna fix this.

I think this had something to do with me merging my edits with the bot edits; I just ran the pre-commit, and it edited the file automatically and passed once I ran it again. I'll push again with the pre-commit's edit.

@danielsirakov

Copy link
Copy Markdown
Author

@stevenhua ready to review

@stevenhua0320

Copy link
Copy Markdown
Contributor

@danielsirakov Still not passing, first you could do the git pull origin <branch-name> to merge the current pre-commit bot update, then do the single black and docformatter sequentially and see what did they edit on the same file.

@danielsirakov

Copy link
Copy Markdown
Author

@danielsirakov Still not passing, first you could do the git pull origin <branch-name> to merge the current pre-commit bot update, then do the single black and docformatter sequentially and see what did they edit on the same file.

Sorry about that, it said it was passing when I ran the pre-commit before pushing so I’m not sure why that’s happening. I’m not home right now, but I’ll take a look as soon as I’m back.

@sbillinge sbillinge left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left comments. Let's try moving the failing module level docstrings to the top

Comment thread src/diffpy/srfit/exceptions.py Outdated
Exceptions used for SrFit - specific errors.
"""

"Exceptions used for SrFit - specific errors."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should be a docstring I think. Does it work putting it right at the top of the file but with triple quotes?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay; I've been having technical difficulties with my computer. When I try this, the docformatter and black conflict happens once again. Are there any other workarounds you recommend I should try?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you describe how does it conflict with each other, in other words, what does black do initially?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like black is adding a blank line before class, while docformatter is removing it

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the same scenario that we had before, try to add a #FIXME comment and see if we could get around this.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just pushed, let me know if everything looks fine

@stevenhua0320 ready to review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more fix is needed, look into the tests/test_characteristicfunctions.py and see how does black and docformatter is conflicting right now. Also don't stay up too late haha.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look right now. Don't worry, I'm gonna hit the hay after this : )

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looked to be the same scenario as before, and I added a #FIXME line. Let me know if it looks fine

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, it looks like black and the pre-commit-ci bot have conflict, as the bot removes a line that black adds. What do you recommend we do? Claude recommended I move the import to the top of the file instead of inside the function.

Comment thread src/diffpy/srfit/sas/sasimport.py Outdated
#
##############################################################################
"""Universal import functions for volatile SasView/SansViews API-s."""
"Universal import functions for volatile SasView/SansViews API-s."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a docstring, try moving to the top

Comment thread src/diffpy/srfit/util/argbinders.py Outdated
#
##############################################################################
"""Functions for binding arguments of callable objects."""
"Functions for binding arguments of callable objects."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring

+ "latest sasview API"
)
from sasmodels.sasview_model import find_model, load_standard_models

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we move this import statement to the top of import modules and check whether it could fix the conflicting behavior while the test module is still valid?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved it to the top but I noticed there are other instances of the import statements being in that location, should I delete them all since I have it in the import section now?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, how do I check that the test module is still valid?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they import the same function, you could remove it and test it by running pytest on your terminal; otherwise, you need to add the function name eg:find_model after the import and rerun pytest to see whether the test is running properly.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble running pytest as I don't have the necessary packages downloaded in my environment, and when I tried downloading them, it said that the diffpy-srfit package required Python between 3.11 and 3.13. Should I make a new environment, or try and test it in a different way?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I'll finish this tomorrow as I'm going to bed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I'll finish this tomorrow as I'm going to bed

No worries, have a good rest for today!

@danielsirakov danielsirakov Jun 22, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! You too!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to have this installed in editable mode to do testing, but this is standard practice, we should maybe have told you. I also am seeing the CI pre-commit working target hard. Out Is Better of you are running pre-commit locally. We should only see CI pre-commit things very rarely when things go wrong locally. @Rundong can help you get everything set up locally correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants